New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support unqualified and partially qualified domain name in DNS query in Windows kube-proxy #41618

Merged

k8s-github-robot merged 4 commits into kubernetes:master from JiangtianLi:k8swin

Mar 1, 2017

Contributor

JiangtianLi commented Feb 17, 2017 •

edited

What this PR does / why we need it:
In Windows container networking, --dns-search is not currently supported on Windows Docker. Besides, even with --dns-suffix, inside Windows container DNS suffix is not appended to DNS query names. That makes unqualified domain name or partially qualified domain name in DNS query not able to resolve.

This PR provides a solution to resolve unqualified domain name or partially qualified domain name in DNS query for Windows container in Windows kube-proxy. It uses well-known Kubernetes DNS suffix as well host DNS suffix search list to append to the name in DNS query. DNS packet in kube-proxy UDP stream is modified as appropriate.

This PR affects the Windows kube-proxy only.

Special notes for your reviewer:
This PR is based on top of Anthony Howe's commit 48647fb, 0e37f0a and 7e2c71f which is already included in the PR 41487. Please only review commit b9dfb69.

Release note:

Add DNS suffix search list support in Windows kube-proxy.

Contributor

k8s-ci-robot commented Feb 17, 2017

Hi @JiangtianLi. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Contributor

k8s-ci-robot commented Feb 17, 2017

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot added the cncf-cla: no label

k8s-reviewable commented Feb 17, 2017

This change is

k8s-github-robot assigned brendandburns

k8s-github-robot added size/XL release-note-label-needed labels

Contributor Author

JiangtianLi commented Feb 17, 2017

I signed it! Thanks.

brendandburns reviewed

View reviewed changes

pkg/proxy/winuserspace/proxier.go Outdated

    
            @@ -28,7 +27,7 @@ import (
          
              	"github.com/golang/glog"

              	"k8s.io/apimachinery/pkg/types"

              	utilerrors "k8s.io/apimachinery/pkg/util/errors"

              	//utilerrors "k8s.io/apimachinery/pkg/util/errors"

Contributor

brendandburns Feb 20, 2017

Delete this if its no longer used.

Contributor Author

JiangtianLi Feb 22, 2017

Sorry for the confusion. My commit is based on Anthony's commit 302fa44 and 1410e06 so I had to include his commits here. In previous PR, the second commit 1410e06 was lost when I merge his and squash my commits. Now I did rebase and it looked fine. Please skip reviewing diff in commit 302fa44 and 1410e06.

brendandburns reviewed

View reviewed changes

pkg/proxy/winuserspace/proxier.go Outdated

    
              	timeout       time.Duration

              	activeClients *clientCache

              	dnsClients    *dnsClientCache

              	//nodePort      int

Contributor

brendandburns Feb 20, 2017

delete these if the are no longer used.

brendandburns reviewed

View reviewed changes

pkg/proxy/winuserspace/proxier.go Outdated

    
              	netsh          netsh.Interface

              	hostIP         net.IP

              	proxyPorts     PortAllocator

              	//listenIP       net.IP

Contributor

brendandburns Feb 20, 2017

here too.

brendandburns reviewed

View reviewed changes

pkg/proxy/winuserspace/proxier.go Outdated

    
              	//listenIP       net.IP

              	netsh  netsh.Interface

              	hostIP net.IP

              	//proxyPorts     PortAllocator

Contributor

brendandburns Feb 20, 2017

and here

brendandburns suggested changes

View reviewed changes

pkg/proxy/winuserspace/proxier.go Outdated

    
            @@ -196,60 +188,64 @@ func (proxier *Proxier) SyncLoop() {
          
              // Ensure that portals exist for all services.

              func (proxier *Proxier) ensurePortals() {

              	proxier.mu.Lock()

              	/*proxier.mu.Lock()

Contributor

brendandburns Feb 20, 2017

delete this block.

pkg/proxy/winuserspace/proxier.go Outdated

    
              		spn := proxy.ServicePortName{NamespacedName: types.NamespacedName{Namespace: k.Namespace, Name: k.Name}, Port: k.Port}

              		spnMap[spn] = true

              	}

              	for name := range spnMap {

Contributor

brendandburns Feb 20, 2017

This indirection seems weird, why not just proxier.loadBalancer.CleanupStaleStickySessions(name) where you set spnMap[spn]=true

pkg/proxy/winuserspace/proxier.go Outdated

    
              			var listenIPs []string

              			listenIPs = append(listenIPs, service.Spec.ClusterIP)

              			/*for _, ip := range service.Spec.ExternalIPs {

Contributor

brendandburns Feb 20, 2017

delete dead code.

pkg/proxy/winuserspace/proxier.go Outdated

    
              					// turn off the proxy

              					err := proxier.stopProxy(serviceName, info)

              					if err != nil {

              						glog.Errorf("Failed to stop service %q: %v", serviceName, err)

Contributor

brendandburns Feb 20, 2017

bail out here?

pkg/proxy/winuserspace/proxier.go Outdated

    
              							glog.Errorf("Failed to delete IP address for service %q, error %s", serviceName, err.Error())

              						}

              					} else {

              						// TODO(ajh) release the node port

Contributor

brendandburns Feb 20, 2017

Address this todo?

pkg/proxy/winuserspace/proxier.go Outdated

    
              				}

              				si := &serviceInfo{

              					isAliveAtomic:       1,

              					portal:              portal{ip: listenIP, port: int(servicePort.Port), isExternal: false},

Contributor

brendandburns Feb 20, 2017

Multi-line initializer.

pkg/proxy/winuserspace/proxier.go Outdated

    
              	// only delete spn if all listen ips show inactive

              	for k := range spnMap {

              		if !spnMap[k] {

              			proxier.loadBalancer.DeleteService(k)

Contributor

brendandburns Feb 20, 2017

As above seems like you could inline this in the previous loop.

pkg/proxy/winuserspace/proxier.go Outdated

    
              	if info.protocol != port.Protocol || info.portal.port != int(port.Port) || info.nodePort != int(port.NodePort) {

              		return false

              	}

              	if !info.portal.ip.Equal(net.ParseIP(service.Spec.ClusterIP)) {

Contributor

brendandburns Feb 20, 2017

Why did you delete all of this?

pkg/proxy/winuserspace/proxier.go Outdated

    
            @@ -402,6 +459,7 @@ func ipsEqual(lhs, rhs []string) bool {
          
              	return true

              }

              /*

Contributor

brendandburns Feb 20, 2017

Delete commented out code.

pkg/proxy/winuserspace/proxysocket.go

    
              func (udp *udpProxySocket) ProxyLoop(service proxy.ServicePortName, myInfo *serviceInfo, proxier *Proxier) {

              // TODO: use Go net dnsmsg library to walk DNS message format

              // DNS packet header

              type dnsHeader struct {

Contributor

brendandburns Feb 20, 2017

This code could really use some unit tests.

Contributor Author

JiangtianLi Feb 22, 2017

Thanks for the suggestion. I will add unit tests.

Contributor Author

JiangtianLi Feb 23, 2017

I added unit tests in a new commit.

JiangtianLi force-pushed the k8swin branch from e78a5d2 to eceb5e2 Compare

February 21, 2017 20:14

k8s-ci-robot added cncf-cla: yes and removed cncf-cla: no labels

JiangtianLi force-pushed the k8swin branch from eceb5e2 to 29c55bc Compare

February 22, 2017 01:10

k8s-github-robot added size/XXL and removed size/XL labels

anhowe added 2 commits

February 22, 2017 19:42


          add tcp or udp proxy for service addresses

48647fb


          cleanup proxier

0e37f0a

Contributor

brendandburns commented Feb 28, 2017

/lgtm
/approve

k8s-ci-robot added the lgtm label

k8s-github-robot added approved do-not-merge labels

Contributor

brendandburns commented Feb 28, 2017

@k8s-bot bazel test this

JiangtianLi force-pushed the k8swin branch from a436020 to ae07913 Compare

February 28, 2017 01:08

k8s-github-robot removed the lgtm label

anhowe and others added 2 commits

February 28, 2017 02:56


          per Jenkin's test instructions run Run ./hack/update-bazel.sh

7e2c71f


          Fix DNS suffix search list issue for Windows container and workaround…

b9dfb69

… in kube-proxy.

kube-proxy iterates over DNS suffix search list and appends to DNS query for client.

JiangtianLi force-pushed the k8swin branch from ae07913 to b9dfb69 Compare

February 28, 2017 03:26

Contributor

brendandburns commented Feb 28, 2017

@JiangtianLi can you add a descriptive title for this PR (e.g. "support dns search in windows proxy")

Thanks
--brendan

JiangtianLi changed the title ~~K8swin~~ Support unqualified and partially domain name in DNS query in Windows kube-proxy

Contributor Author

JiangtianLi commented Feb 28, 2017

@brendandburns Thanks for the reminder. I have modified the title.

Contributor

michmike commented Feb 28, 2017

cc: @pires as well for a review

k8s-github-robot added release-note and removed release-note-label-needed labels

brendandburns removed the do-not-merge label

Contributor

brendandburns commented Feb 28, 2017

/lgtm

k8s-ci-robot added the lgtm label

brendandburns approved these changes

View reviewed changes

JiangtianLi changed the title ~~Support unqualified and partially domain name in DNS query in Windows kube-proxy~~ Support unqualified and partially qualified domain name in DNS query in Windows kube-proxy

k8s-github-robot commented Feb 28, 2017

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: JiangtianLi, brendandburns, k8s-merge-robot

Needs approval from an approver in each of these OWNERS Files:

~~cmd/OWNERS~~ [brendandburns]
~~pkg/OWNERS~~ [brendandburns]
~~pkg/proxy/OWNERS~~ [brendandburns]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

Contributor

brendandburns commented Feb 28, 2017

@k8s-bot kops aws e2e test this

brendandburns added this to the v1.6 milestone

k8s-github-robot commented Mar 1, 2017

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

Contributor

k8s-ci-robot commented Mar 1, 2017 •

edited

@JiangtianLi: The following test(s) failed:

Test name	Commit	Details	Rerun command
Jenkins GCE etcd3 e2e	`b9dfb69`	link	`@k8s-bot gce etcd3 e2e test this`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-github-robot commented Mar 1, 2017

Automatic merge from submit-queue (batch tested with PRs 42316, 41618, 42201, 42113, 42191)

k8s-github-robot merged commit 44a7be9 into kubernetes:master

JiangtianLi deleted the k8swin branch

April 7, 2017 16:57

This was referenced Apr 24, 2017

Fix the issue in Windows kube-proxy when processing unqualified name. This is for DNS client such as ping or iwr that validate name in response and original question. #44869

Closed

Fix the issue in Windows kube-proxy when processing unqualified name. This is for DNS client such as ping or iwr that validate name in response and original question. #44872

Closed

JiangtianLi mentioned this pull request

Fix the issue in Windows kube-proxy when processing unqualified name. This is for DNS client such as ping or iwr that validate name in response and original question. #45642

Merged

k8s-github-robot pushed a commit that referenced this pull request


          Merge pull request #45642 from JiangtianLi/jiangtli-fixdns2

f706b81

Automatic merge from submit-queue

Fix the issue in Windows kube-proxy when processing unqualified name. This is for DNS client such as ping or iwr that validate name in response and original question.

**What this PR does / why we need it**:
This PR is an additional fix to #41618 and [the corresponding commit](b9dfb69). The DNS client such as nslookup does not validate name matching in response and original question. That works fine when we append DNS suffix to unqualified name in DNS query in Windows kube-proxy. However, for DNS client such as ping or Invoke-WebRequest that validates name in response and original question, the issue arises and the DNS query fails although the received DNS response has no error. 

This PR fixes the additional issue by restoring the original question name in DNS response. Further, this PR refactors DNS message routines by using miekg's DNS library.

This PR affects the Windows kube-proxy only.

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #42605 

**Special notes for your reviewer**:

**Release note**:

```release-note
Fix DNS suffix search list support in Windows kube-proxy.
```

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment